Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: precision issues around price; short and long term solution #5552

Merged
merged 10 commits into from
Jun 19, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Jun 16, 2023

Closes: #XXX

What is the purpose of the change

Related to: #5550

Documenting latest decisions around tick and price conversions

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@p0mvn p0mvn added the F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board label Jun 16, 2023
@github-actions github-actions bot added C:docs Improvements or additions to documentation C:x/concentrated-liquidity labels Jun 16, 2023
@p0mvn p0mvn marked this pull request as ready for review June 16, 2023 20:16
@p0mvn p0mvn requested a review from czarcas7ic as a code owner June 16, 2023 20:16
x/concentrated-liquidity/README.md Outdated Show resolved Hide resolved
x/concentrated-liquidity/README.md Outdated Show resolved Hide resolved
x/concentrated-liquidity/README.md Outdated Show resolved Hide resolved
x/concentrated-liquidity/README.md Show resolved Hide resolved
x/concentrated-liquidity/README.md Outdated Show resolved Hide resolved
x/concentrated-liquidity/README.md Outdated Show resolved Hide resolved
Comment on lines +1675 to +1681
Now, we have a problem that we cannot handle pairs where
the quote asset has a precision of 6 and the base asset has a
precision of 18.

Note that this is not a problem for pairs where the quote asset
has a precision of 18 and the base asset has a precision of 6.
E.g. OSMO/DAI.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm trying to understand this a little more

so in the above example,

osmo quote (6th precision) & arb base (18th prevision)   
spot price = (218023341414 / 10**6) / (101170077995723619690981 / 10**18) 
spot price = 2.1550180224553714

Why would this not work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in AMM units, this is:

>>> 218023341414 / 101170077995723619690981
2.1550180224553714e-12

It does not account for precision. As a result, we are right at the boundary of the spot price that can be represented without issues

Copy link
Contributor

@stackman27 stackman27 Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see, new min limit is 10^-12.

but why don't we do (quote / precision) / (base / precision) instead of just quote / base, on chain side?

Copy link
Member Author

@p0mvn p0mvn Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how precisions for these assets were historically defined. So we must normalize them to be in the same units

@p0mvn
Copy link
Member Author

p0mvn commented Jun 18, 2023

Addressed all comments. R4R

Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for adding this!

@mergify mergify bot merged commit 5008d7b into main Jun 19, 2023
@mergify mergify bot deleted the roman/precision-issues-docs branch June 19, 2023 13:12
mattverse pushed a commit that referenced this pull request Jun 20, 2023
)

Closes: #XXX

## What is the purpose of the change

Related to: #5550

Documenting latest decisions around tick and price conversions

## Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

## Documentation and Release Note

  - [ ] Does this pull request introduce a new feature or user-facing behavior changes?
  - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`?

Where is the change documented? 
  - [ ] Specification (`x/{module}/README.md`)
  - [ ] Osmosis documentation site
  - [ ] Code comments?
  - [ ] N/A
ValarDragon added a commit that referenced this pull request Jun 20, 2023
* Some optimizations

* Update x/concentrated-liquidity/math/math.go

Co-authored-by: Dev Ojha <[email protected]>

* [CL]: Fix incorrect bound check/chain halt vector (#5557)

* repro panic trigger and fix bound check

* fix comments

* docs: precision issues around price; short and long term solution (#5552)

Closes: #XXX

## What is the purpose of the change

Related to: #5550

Documenting latest decisions around tick and price conversions

## Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

## Documentation and Release Note

  - [ ] Does this pull request introduce a new feature or user-facing behavior changes?
  - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`?

Where is the change documented? 
  - [ ] Specification (`x/{module}/README.md`)
  - [ ] Osmosis documentation site
  - [ ] Code comments?
  - [ ] N/A

* Make go tests only run if relevant (#5569)

* CL: migration functional test (#5560)

Closes: #XXX

## What is the purpose of the change

The following PR introduces a functional test that:
- Creates every type of balancer position that can be created
  - Bonded superfluid
  - Unbonded superfluid (locked)
  - Unbonded superfluid (unlocking)
  - Vanilla lock (locked)
  - Vanilla lock (unlocking)
  - No lock
- It then migrates each position, asserting invariants after each position is migrated
- Finally, an overall invariant is run after all positions have been migrated, asserting that all funds are accounted for in some way

The next PR subsequent to this will be adding randomization to the inputs in order to make the test non deterministic. 

## Testing and Verifying

Functional test above added

## Documentation and Release Note

  - [ ] Does this pull request introduce a new feature or user-facing behavior changes?
  - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`?

Where is the change documented? 
  - [ ] Specification (`x/{module}/README.md`)
  - [ ] Osmosis documentation site
  - [ ] Code comments?
  - [ ] N/A

* [CL]: Fix tick rounding bug and implement direct conversion from tick <> sqrt price (#5541)

Closes: #5516

> **Note to reviewer:** the only real state machine change is in `tick.go` and heavily mirrors @ValarDragon's PR here: #5522 
> The rest of the changes are related to function renames/test refactors.

## What is the purpose of the change

This PR expands on #5522 and updates all price to tick conversions to use SqrtPriceToTick instead.

## Testing and Verifying

The new function is tested in `math/tick_test.go`

The original attack vector is also converted into an edge case test in `position_test.go`

## Documentation and Release Note

  - [ ] Does this pull request introduce a new feature or user-facing behavior changes?
  - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`?

Where is the change documented? 
  - [ ] Specification (`x/{module}/README.md`)
  - [ ] Osmosis documentation site
  - [ ] Code comments?
  - [ ] N/A

* Still fixing merge conlfict

* Revert lp.go change as commented

---------

Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: alpo <[email protected]>
Co-authored-by: Roman <[email protected]>
Co-authored-by: Adam Tucker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge A:no-changelog C:docs Improvements or additions to documentation C:x/concentrated-liquidity F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants